Fix copy link to logs functionality #15368

timroes · 2022-08-05T15:11:32Z

What

This fixes the problem that linking to an individual log did not open that log or scroll to it anymore. The issue was that we tried to compare a string to a number, which typescript didn't see due to us casting in the getJobId method and therefore destroying the typesafteyness.

I've also removed the unused shortInfo property. I checked if we forgot to remove it when we didn't use that feature anymore, but it seems it got introduced in https://github.com/airbytehq/airbyte/pull/9585/files and simply was never used.

Since I got into some confusion around the StatusIcon component, I also made sure that error can be an explicitly passed in state now, to clarify what happens when you'd prior just use <StatusIcon /> (this is slightly unrelated to the rest and if someone has an issue with having that in the same PR, I can separate it into two. Since both changes were really small, I had them in one for now.)

timroes · 2022-08-05T15:12:03Z

airbyte-webapp/src/components/JobItem/JobItem.tsx

+export const getJobStatus: (
+  job: SynchronousJobReadWithStatus | JobsWithJobs
+) => JobStatus | CheckConnectionReadStatus = (job) => {
+  return "status" in job ? job.status : job.job.status;


ℹ️ The casting also caused TypeScript to lose the real return type here, so changed this as well.

timroes · 2022-08-05T15:12:57Z

airbyte-webapp/src/components/JobItem/JobItem.tsx

 };

 export const getJobAttemps: (job: SynchronousJobReadWithStatus | JobsWithJobs) => AttemptRead[] | undefined = (job) => {
  return "attempts" in job ? job.attempts : undefined;
 };

-export const getJobId = (job: SynchronousJobReadWithStatus | JobsWithJobs) =>
-  (job as SynchronousJobReadWithStatus).id ?? (job as JobsWithJobs).job.id;
+export const getJobId = (job: SynchronousJobReadWithStatus | JobsWithJobs) => ("id" in job ? job.id : job.job.id);


ℹ️ Doing this without casting now causes this method to understand that its return type is number | string.

When I was initially going through here I thought there was a bug. But no, we unfortunately get back both from the backend :/

timroes · 2022-08-05T15:13:09Z

airbyte-webapp/src/components/JobItem/JobItem.tsx

  const { jobId: linkedJobId } = useAttemptLink();
-  const [isOpen, setIsOpen] = useState(linkedJobId === getJobId(job));
+  const [isOpen, setIsOpen] = useState(linkedJobId === String(getJobId(job)));


ℹ️ This is the actual fix, that restores that behavior.

timroes · 2022-08-05T15:13:35Z

airbyte-webapp/src/components/JobItem/JobItem.tsx

@@ -32,28 +32,28 @@ const LoadLogs = styled.div`
 `;

 interface JobItemProps {
-  shortInfo?: boolean;


ℹ️ This was never passed into this component, so removed it.

timroes · 2022-08-05T15:14:48Z

airbyte-webapp/src/components/StatusIcon/StatusIcon.tsx

@@ -20,20 +20,22 @@ interface StatusIconProps {

 const getBadgeWidth = (props: StatusIconProps) => (props.big ? (props.value ? 57 : 40) : props.value ? 37 : 20);

-const _iconByStatus: Partial<Record<StatusIconStatus, IconDefinition | undefined>> = {
+const _iconByStatus = {


ℹ️ By changing this just to a as const, TypeScript understands which keys exactly exist in this, unless beforehand where it always suspected accessing something in here could also be undefined.

timroes · 2022-08-05T15:15:38Z

airbyte-webapp/src/components/StatusIcon/StatusIcon.tsx

@@ -44,8 +46,8 @@ const Container = styled.div<StatusIconProps>`
  vertical-align: middle;
 `;

-const Badge = styled(Container)<StatusIconProps>`
-  background: ${(props) => props.theme[(props.status && _themeByStatus[props.status]) || "dangerColor"]};
+const Badge = styled(Container)<{ status: Exclude<StatusIconStatus, "loading"> }>`


ℹ️ Just require the properties that are actually used here (and in the Pick above), can simplify that code, since it understands that theme[...] cannot be undefined anymore.

timroes · 2022-08-05T15:16:07Z

airbyte-webapp/src/components/StatusIcon/StatusIcon.tsx

@@ -66,7 +68,7 @@ const Value = styled.span`
  padding-left: 3px;
 `;

-const StatusIcon: React.FC<StatusIconProps> = ({ title, status, ...props }) => {
+const StatusIcon: React.FC<StatusIconProps> = ({ title, status = "error", ...props }) => {


ℹ️ Make error the default status, to not need to adjust all places this was used without a status.

edmundito

Tested it locally and the job expands but for some reason it doesn't scroll as implemented in

airbyte/airbyte-webapp/src/components/JobItem/JobItem.tsx

Lines 65 to 72 in 4d84bdb

    
           useEffectOnce(() => { 
        
             if (linkedJobId) { 
        
               scrollAnchor.current?.scrollIntoView({ 
        
                 behavior: "smooth", 
        
                 block: "start", 
        
               }); 
        
             } 
        
           });

airbyte-webapp/src/components/JobItem/JobItem.tsx

Co-authored-by: Edmundo Ruiz Ghanem <168664+edmundito@users.noreply.github.com>

timroes · 2022-08-08T14:34:24Z

@edmundito thanks for finding this. There was one small issue within the if, but the problem also is, that this doesn't work calling this without any timout, since the page hasn't finished rendering. I've played a bit around with timeouts and you need unfortunately to get up to a second to delay to make this work somewhat reliable. So I added a second delay before scrolling to the element. Alternatively if we consider this solution to be too hacky, we could remove the scrolling code altogether.

krishnaglick · 2022-08-08T14:38:17Z

airbyte-webapp/src/components/JobItem/JobItem.tsx

 };

 export const getJobAttemps: (job: SynchronousJobReadWithStatus | JobsWithJobs) => AttemptRead[] | undefined = (job) => {
  return "attempts" in job ? job.attempts : undefined;
 };

-export const getJobId = (job: SynchronousJobReadWithStatus | JobsWithJobs) =>
-  (job as SynchronousJobReadWithStatus).id ?? (job as JobsWithJobs).job.id;
+export const getJobId = (job: SynchronousJobReadWithStatus | JobsWithJobs) => ("id" in job ? job.id : job.job.id);


When I was initially going through here I thought there was a bug. But no, we unfortunately get back both from the backend :/

krishnaglick · 2022-08-08T14:39:34Z

airbyte-webapp/src/components/JobItem/JobItem.tsx

    }
+    return undefined;


https://reactjs.org/docs/hooks-reference.html#uselayouteffect
?

This unfortunately doesn't help (I barely ever had a case where useLayoutEffect actually helped :D). This will just cause it to slip into the execution slot after the next rendering, which unfortunately isn't enough in this case. There is just too much (async) rendering still ongoing, that even setting a timeout of like 300ms isn't enough to get this work. Tested also with useLayoutEffect and it does neither scroll (without the high timeout).

I don't think it's a better idea, but could do a setInterval(() => {}, 100) that runs until it's loaded.

We unfortunately also don't have a way to determine whether it's loaded in that sense. The DOM reference IS there from thebeginning and mounted, so there is no clear way to determine that the browser is done with rendering everything around it.

We do!

Can fiddle with exact implementation, but I think this is a nice solution no?

Oh good call. Yeah we def can use the animationComplete there to trigger the scroll to. Will change this tomorrow.

I can push this if you'd like

Pushed to kc/fix-link-to-logs

Added this to this PR, as discussed offline this won't work in Chrome (on Linux) apparently, but since it'll work in Firefox stable, we'll still keep this code in.

airbyte-webapp/src/components/JobItem/JobItem.tsx

edmundito

Tested locally.

commit 10fb1dc137175d09826cdfcf419698e3000cd418 Author: pmossman <parker@airbyte.io> Date: Wed Aug 10 13:37:00 2022 -0700 format and pmd commit 7c223ec2e0abfec864c11395d7310d679706a49c Author: pmossman <parker@airbyte.io> Date: Wed Aug 10 13:08:09 2022 -0700 update peristStateActivity test commit 763e9e2c5ca5f998ab49ffd0dcafb7ae81201b2b Author: pmossman <parker@airbyte.io> Date: Fri Aug 5 15:24:10 2022 -0700 format commit c176e63c3841a1f08c7a43359d293b12297e03e4 Author: pmossman <parker@airbyte.io> Date: Fri Aug 5 15:18:03 2022 -0700 move converters to module that worker can access, convert statePersistence calls to API calls, convert statePersistence helper to local private method commit 1b583487b4ea7dd058944cdbce4de6197f967523 Author: pmossman <parker@airbyte.io> Date: Fri Aug 5 10:37:00 2022 -0700 add createOrUpdateState API endpoint commit d87eed6215ce451a3e126d433991967317839876 Author: pmossman <parker@airbyte.io> Date: Fri Aug 5 13:42:16 2022 -0700 add AirbyteApiClient to WorkerApp for data plane workers to use commit a65524a Author: Teal Larson <LARSON.TEAL@GMAIL.COM> Date: Wed Aug 10 16:03:59 2022 -0400 🪟 🔧 Add testing and storybook component for CatalogDiffModal (#15426) * wip diff modal test setup * starting storybook add * storybook working now * cleanup * aria labels * test syncmode string commit 2f17e99 Author: Liren Tu <tuliren.git@outlook.com> Date: Wed Aug 10 13:02:01 2022 -0700 🐞 Postgres source: fix bug in intermediate state emission (#15496) * Rename record counter * Rename method * Emit intermediate state after all cursor records * Emit intermediate state only when it is ready * Merge two checks * Add a testing message * Fix unit tests * Add one more testing record and add comments * Add test case for multiple records with the same cursor value * Revert irrelevant change * Add explanation in javadoc * Format code * Rename testing methods * Fix comment * Bump version * auto-bump connector version [ci skip] Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com> commit f540499 Author: Alexandre Girard <alexandre@airbyte.io> Date: Wed Aug 10 11:37:07 2022 -0700 [low-code connectors]: Assert there are no custom top-level fields (#15489) * move components to definitions field * Also update the references * validate the top level fields and add version * raise exception on unknown fields * newline * unit tests * set version to 0.1.0 * newline commit f52bfb6 Author: Xiaohan Song <xiaohan@airbyte.io> Date: Wed Aug 10 11:16:17 2022 -0700 change query frequency to 1hour (#15499) commit f143c8f Author: midavadim <midavadim@yahoo.com> Date: Wed Aug 10 21:13:51 2022 +0300 :tada: Source File - add support for custom encoding (#15293) * added support for custom encoding * fixed unit test for utf16 * updated docs * bumped connector version * auto-bump connector version [ci skip] Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com> commit bbf3584 Author: Alexandre Girard <alexandre@airbyte.io> Date: Wed Aug 10 10:58:22 2022 -0700 Remove unused field from JsonSchema (#15425) * few fixes from working with sendgrid * reset to master * only update the docstring * reset commit a280113 Author: VitaliiMaltsev <39538064+VitaliiMaltsev@users.noreply.github.com> Date: Wed Aug 10 20:44:51 2022 +0300 Destination S3: add LZO compression support (#15394) * Fixed bucket naming for S3 * Destination S3: add LZO compression support for parquet files * Destination S3: add LZO compression support for parquet files * implemented logic for aarch64 * removed redundant logging * updated changelog * moved intstall of native-lzo lib to Dockerfile * removed redundant logging * add unit test for aarch64 * bump version * auto-bump connector version [ci skip] Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com> commit 29c3426 Author: sivankumar86 <sivankumar86@users.noreply.github.com> Date: Thu Aug 11 02:34:47 2022 +1000 Source MSSQL: special character support in dbname for CDC method (#15430) * information schema included * special character handle * Revert "information schema included" This reverts commit f0aee6a. * version change * doc update * auto-bump connector version [ci skip] Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com> commit 959d862 Author: Baz <oleksandr.bazarnov@globallogic.com> Date: Wed Aug 10 19:20:22 2022 +0300 🐛 Source SalesForce: changed `DEFAULT_WAIT_TIMEOUT_SECONDS` to 24-hour limit (#15444) commit 0092712 Author: Subodh Kant Chaturvedi <subodh1810@gmail.com> Date: Wed Aug 10 21:48:57 2022 +0530 fix postgres data handling from WAL logs in CDC mode (#15481) * fix postgres data handling from WAL logs in CDC mode * format * use formatter for dates also (#15485) * format * change test structure * change log to debug Co-authored-by: Edward Gao <edward.gao@airbyte.io> commit fdb5eb9 Author: Evan Tahler <evan@airbyte.io> Date: Wed Aug 10 09:03:02 2022 -0700 Simplify the `MigrationAcceptanceTest` (#15497) * disable `testAutomaticMigration` * empty commit to retry tests * Simplify the MigrationAcceptanceTest * lint * Fix PMD. Reorder some calls to make clear what is happening. Co-authored-by: Davin Chia <davinchia@gmail.com> commit fd70913 Author: Augustin <augustin.lafanechere@gmail.com> Date: Wed Aug 10 17:42:07 2022 +0200 SAT: compatibility tests for catalogs (#15486) commit 1ad5152 Author: Evan Tahler <evan@airbyte.io> Date: Wed Aug 10 08:21:52 2022 -0700 Disable automaticMigrationAcceptanceTest (#15492) * disable `testAutomaticMigration` * empty commit to retry tests commit 1228451 Author: Edmundo Ruiz Ghanem <168664+edmundito@users.noreply.github.com> Date: Wed Aug 10 11:11:45 2022 -0400 Fix styles impacting global ul, li in FieldSection component (#15484) commit 6aa08e0 Author: Jonathan Pearlin <jonathan@airbyte.io> Date: Wed Aug 10 10:55:46 2022 -0400 Add micronaut dependencies and bundles (#15459) * Add micronaut dependencies and bundles * Update Micronaut core commit 7662956 Author: Edmundo Ruiz Ghanem <168664+edmundito@users.noreply.github.com> Date: Wed Aug 10 10:51:43 2022 -0400 🪟 🧹 Cleanup documentation panel components (#15455) * Add docs/ to frontend workspace * Migrate Markdown components to scss and cleanup when not found is rendered * Add white-space: break-spaces rule to markdown code blocks commit 1258ab4 Author: Topher Lubaway <asimplechris@gmail.com> Date: Wed Aug 10 09:05:14 2022 -0500 Revert "Adds PAT check to shared pr check (#15453)" (#15511) This reverts commit 06a18d4. commit 853b88a Author: Kyryl Skobylko <xpuska513@gmail.com> Date: Wed Aug 10 16:48:20 2022 +0300 fix: fix gcs-log creds secret name, add externaldb configuration for temporal, fix webapp ingress (#15510) commit c782303 Author: Yatsuk Bogdan <yatsukbogdan@gmail.com> Date: Wed Aug 10 15:57:26 2022 +0300 :window: :art: Increases GroupTitle followed divs width from 180px to 250px (#13956) * Increases GroupControls followed divs width from 180px to 250px * Increases min-width for GroupTitle * Change layout to flexbox Co-authored-by: Tim Roes <tim@airbyte.io> commit e28bc3a Author: Serhii Chvaliuk <grubberr@gmail.com> Date: Wed Aug 10 13:55:29 2022 +0300 🎉Source Harvest: Added `parent_id` for all streams which have parent stream (#15221) Signed-off-by: Sergey Chvalyuk <grubberr@gmail.com> commit aaa3aae Author: Tuhai Maksym <kimerinn@gmail.com> Date: Wed Aug 10 12:43:55 2022 +0300 15310: Destination Scylla: Handle per-stream state (#15399) * 15310: Destination Scylla: Handle per-stream state * 15399: test fix * 15318: test fix * 15318: updating version * auto-bump connector version [ci skip] Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com> commit c724630 Author: Yurii Bidiuk <35812734+yurii-bidiuk@users.noreply.github.com> Date: Wed Aug 10 10:17:23 2022 +0300 Add test case for new fields appearing in data (#15372) * add test case for new field(s) appearing in data * rework test to verify that sync at least not failed if new fields are present commit 6e1a76f Author: Serhii Chvaliuk <grubberr@gmail.com> Date: Wed Aug 10 09:24:40 2022 +0300 🐛 Source Amazon Ads: define primary_key for all report streams (#15469) Signed-off-by: Sergey Chvalyuk <grubberr@gmail.com> commit c1a0cbc Author: Octavia Squidington III <90398440+octavia-squidington-iii@users.noreply.github.com> Date: Wed Aug 10 04:20:12 2022 +0200 Bump Airbyte version from 0.39.42-alpha to 0.40.0-alpha (#15493) Co-authored-by: benmoriceau <benmoriceau@users.noreply.github.com> commit f6766ee Author: Benoit Moriceau <benoit@airbyte.io> Date: Wed Aug 10 07:50:41 2022 +0800 Revert "Revert "Release per stream to the OSS project (#15008)" (#15177)" (#15401) This reverts commit 362fc4e. commit eab0013 Author: Edward Gao <edward.gao@airbyte.io> Date: Tue Aug 9 16:13:09 2022 -0700 🐛 Source snowflake: int columns should be discovered as ints (#15314) * snowflake discovers ints as ints * version bump+changelog * bump version+changelog * auto-bump connector version [ci skip] Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com> commit f506c60 Author: Anne <102554163+alovew@users.noreply.github.com> Date: Tue Aug 9 16:07:35 2022 -0700 Track number of streams in syncs (#15478) * Add number_of_streams to job sync tracking commit 6c5d1ff Author: Augustin <augustin.lafanechere@gmail.com> Date: Wed Aug 10 00:33:58 2022 +0200 SAT: measure unit test coverage (#15443) commit e9afa9b Author: Anne <102554163+alovew@users.noreply.github.com> Date: Tue Aug 9 15:30:48 2022 -0700 Error Prone PMD rules (#15010) * Implement ErrorProne PMD rules: AssignmentInOperand AvoidAccessibilityAlteration AvoidBranchingStatementAsLastInLoop AvoidCatchingNPE AvoidCatchingThrowable AvoidDuplicateLiterals rule commit c536e51 Author: Tim Roes <tim@airbyte.io> Date: Wed Aug 10 00:11:12 2022 +0200 Fix copy link to logs functionality (#15368) * Fix copy link to logs functionality * Update airbyte-webapp/src/components/JobItem/JobItem.tsx Co-authored-by: Edmundo Ruiz Ghanem <168664+edmundito@users.noreply.github.com> * Fix scrolling * Remove smooth scrolling * Improve effect for better return statements * Better scroll Co-authored-by: Edmundo Ruiz Ghanem <168664+edmundito@users.noreply.github.com> commit 62303a8 Author: Augustin <augustin.lafanechere@gmail.com> Date: Tue Aug 9 23:07:13 2022 +0200 SAT: check that previous config schema validates against current connector spec (#15367) commit 123705c Author: Stephen Wentling <stephen@swentling.com> Date: Tue Aug 9 21:30:14 2022 +0100 Source Jira: Added updates to include issue components and fixes to README files (#15135) * solve readme conflict * updated jira sources with open PR details * correct additionalProperties test discover Co-authored-by: marcosmarxm <marcosmarxm@gmail.com> commit 9e691d8 Author: Alex <109167606+alex-gron@users.noreply.github.com> Date: Tue Aug 9 14:28:38 2022 -0500 fix broken link (#15379) commit 36ed6ce Author: Denys Davydov <davydov.den18@gmail.com> Date: Tue Aug 9 21:58:52 2022 +0300 #15445 source typeform: integration tests (#15446) commit 06a18d4 Author: Topher Lubaway <asimplechris@gmail.com> Date: Tue Aug 9 13:33:20 2022 -0500 Adds PAT check to shared pr check (#15453) * Adds PAT check to shared pr check * Name change * Removes "safe_to_push" string * Adds OCTAVIA_PAT and uses the found PAT found PAT was not used in all locales, so this could have still failed on an expired OCTAVIA_PAT before this change

Fix copy link to logs functionality

4d84bdb

timroes requested a review from a team as a code owner August 5, 2022 15:11

github-actions bot added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Aug 5, 2022

timroes commented Aug 5, 2022

View reviewed changes

edmundito reviewed Aug 5, 2022

View reviewed changes

airbyte-webapp/src/components/JobItem/JobItem.tsx Outdated Show resolved Hide resolved

timroes and others added 2 commits August 5, 2022 21:01

Update airbyte-webapp/src/components/JobItem/JobItem.tsx

7aefd6f

Co-authored-by: Edmundo Ruiz Ghanem <168664+edmundito@users.noreply.github.com>

Fix scrolling

4d393d2

Merge branch 'master' into tim/fix-link-to-logs

71e82a3

krishnaglick reviewed Aug 8, 2022

View reviewed changes

edmundito reviewed Aug 8, 2022

View reviewed changes

airbyte-webapp/src/components/JobItem/JobItem.tsx Outdated Show resolved Hide resolved

airbyte-webapp/src/components/JobItem/JobItem.tsx Outdated Show resolved Hide resolved

timroes added 3 commits August 8, 2022 17:51

Remove smooth scrolling

1527d19

Improve effect for better return statements

00f717a

Merge branch 'master' into tim/fix-link-to-logs

f57a686

timroes requested a review from edmundito August 8, 2022 17:32

edmundito approved these changes Aug 9, 2022

View reviewed changes

timroes added 2 commits August 9, 2022 16:38

Merge branch 'master' into tim/fix-link-to-logs

1a18a30

Better scroll

60b0082

timroes merged commit c536e51 into master Aug 9, 2022

timroes deleted the tim/fix-link-to-logs branch August 9, 2022 22:11

octavia-squidington-iii mentioned this pull request Aug 10, 2022

Bump Airbyte version from 0.39.42-alpha to 0.40.0-alpha #15493

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix copy link to logs functionality #15368

Fix copy link to logs functionality #15368

timroes commented Aug 5, 2022

timroes Aug 5, 2022

timroes Aug 5, 2022

krishnaglick Aug 8, 2022

timroes Aug 5, 2022

timroes Aug 5, 2022

timroes Aug 5, 2022

timroes Aug 5, 2022

timroes Aug 5, 2022

edmundito left a comment

timroes commented Aug 8, 2022

krishnaglick Aug 8, 2022

krishnaglick Aug 8, 2022

timroes Aug 8, 2022

krishnaglick Aug 8, 2022

timroes Aug 8, 2022

krishnaglick Aug 8, 2022

timroes Aug 8, 2022

krishnaglick Aug 8, 2022

krishnaglick Aug 8, 2022

timroes Aug 9, 2022

edmundito left a comment

	useEffectOnce(() => {
	if (linkedJobId) {
	scrollAnchor.current?.scrollIntoView({
	behavior: "smooth",
	block: "start",
	});
	}
	});

Fix copy link to logs functionality #15368

Fix copy link to logs functionality #15368

Conversation

timroes commented Aug 5, 2022

What

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edmundito left a comment

Choose a reason for hiding this comment

timroes commented Aug 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edmundito left a comment

Choose a reason for hiding this comment